DAG: Use sincos vector libcalls through RuntimeLibcalls#166984
Conversation
|
@llvm/pr-subscribers-llvm-selectiondag Author: Matt Arsenault (arsenm) ChangesCopy new process from sincospi. Full diff: https://github.com/llvm/llvm-project/pull/166984.diff 3 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/BasicTTIImpl.h b/llvm/include/llvm/CodeGen/BasicTTIImpl.h
index 1c167af4b0478..a52ad41d0f1b3 100644
--- a/llvm/include/llvm/CodeGen/BasicTTIImpl.h
+++ b/llvm/include/llvm/CodeGen/BasicTTIImpl.h
@@ -334,7 +334,12 @@ class BasicTTIImplBase : public TargetTransformInfoImplCRTPBase<T> {
break;
case Intrinsic::sincos:
- LC = RTLIB::getSINCOS(ScalarVT);
+ LC = RTLIB::getSINCOS(VT);
+ if (LC == RTLIB::UNKNOWN_LIBCALL)
+ LC = RTLIB::getSINCOS(ScalarVT);
+ else if (VT.isVector())
+ IsVectorCall = true;
+
break;
default:
return std::nullopt;
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
index f5a54497c8a98..78d8ea0676dd7 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
@@ -1268,10 +1268,12 @@ void VectorLegalizer::Expand(SDNode *Node, SmallVectorImpl<SDValue> &Results) {
return;
break;
-
+ case ISD::FSINCOS:
case ISD::FSINCOSPI: {
EVT VT = Node->getValueType(0);
- RTLIB::Libcall LC = RTLIB::getSINCOSPI(VT);
+ RTLIB::Libcall LC = Node->getOpcode() == ISD::FSINCOS
+ ? RTLIB::getSINCOS(VT)
+ : RTLIB::getSINCOSPI(VT);
if (LC != RTLIB::UNKNOWN_LIBCALL &&
DAG.expandMultipleResultFPLibCall(LC, Node, Results, VT))
return;
@@ -1280,14 +1282,6 @@ void VectorLegalizer::Expand(SDNode *Node, SmallVectorImpl<SDValue> &Results) {
// scalarizing.
break;
}
- case ISD::FSINCOS: {
- // FIXME: Try to directly match vector case like fsincospi
- EVT VT = Node->getValueType(0).getVectorElementType();
- RTLIB::Libcall LC = RTLIB::getSINCOS(VT);
- if (DAG.expandMultipleResultFPLibCall(LC, Node, Results, VT))
- return;
- break;
- }
case ISD::FMODF: {
EVT VT = Node->getValueType(0).getVectorElementType();
RTLIB::Libcall LC = RTLIB::getMODF(VT);
diff --git a/llvm/lib/CodeGen/TargetLoweringBase.cpp b/llvm/lib/CodeGen/TargetLoweringBase.cpp
index 814b4b57a0b9b..b4eb6c357e10e 100644
--- a/llvm/lib/CodeGen/TargetLoweringBase.cpp
+++ b/llvm/lib/CodeGen/TargetLoweringBase.cpp
@@ -425,6 +425,24 @@ RTLIB::Libcall RTLIB::getCOS(EVT RetVT) {
}
RTLIB::Libcall RTLIB::getSINCOS(EVT RetVT) {
+ // TODO: Tablegen should generate this function
+ if (RetVT.isVector()) {
+ if (!RetVT.isSimple())
+ return RTLIB::UNKNOWN_LIBCALL;
+ switch (RetVT.getSimpleVT().SimpleTy) {
+ case MVT::v4f32:
+ return RTLIB::SINCOS_V4F32;
+ case MVT::v2f64:
+ return RTLIB::SINCOS_V2F64;
+ case MVT::nxv4f32:
+ return RTLIB::SINCOS_NXV4F32;
+ case MVT::nxv2f64:
+ return RTLIB::SINCOS_NXV2F64;
+ default:
+ return RTLIB::UNKNOWN_LIBCALL;
+ }
+ }
+
return getFPLibCall(RetVT, SINCOS_F32, SINCOS_F64, SINCOS_F80, SINCOS_F128,
SINCOS_PPCF128);
}
|
96ea8dc to
414d93b
Compare
94e35d8 to
17bfcbe
Compare
| return RTLIB::SINCOS_NXV4F32; | ||
| case MVT::nxv2f64: | ||
| return RTLIB::SINCOS_NXV2F64; | ||
| default: |
There was a problem hiding this comment.
Can this handle SINCOS_V8F64? AMDLIBM has this.
There was a problem hiding this comment.
It could but I'm only handling cases that are tested
There was a problem hiding this comment.
@Andarwinux please raise an issue if you can find AMDLIBM methods llvm doesn't currently handle
There was a problem hiding this comment.
@Andarwinux please raise an issue if you can find AMDLIBM methods llvm doesn't currently handle
No problems. It looks like AMDLIBM is still handling by TLI for now. I thought it would also switch to RuntimeLibcalls soon.
But veclib does indeed have issues on x86, see #164642.
414d93b to
32550a9
Compare
17bfcbe to
404f6f0
Compare
These are the tested set of libcalls used for codegen of llvm.sincos and are needed to get the legalization to follow standard procedure.
Copy new process from sincospi.
404f6f0 to
c5ecd24
Compare
32550a9 to
768d8f2
Compare
|
This PR seems have broken the ability to vectorize sincos with -fveclib, now it no longer vectorizes no matter which veclib is used. see #167871 |
| case VectorLibrary::SLEEFGNUABI: | ||
| for (RTLIB::LibcallImpl Impl : | ||
| {RTLIB::impl__ZGVnN4vl4l4_sincospif, RTLIB::impl__ZGVnN2vl8l8_sincospi, | ||
| {RTLIB::impl__ZGVnN2vl8l8_sincos, RTLIB::impl__ZGVnN4vl4l4_sincosf, |
There was a problem hiding this comment.
It seems this has somehow broken sincos vectorization from the Clang side. Now you have to do: -fveclib=ArmPL -mllvm --vector-library=ArmPL, just -O3 -fno-math-errno -fveclib=ArmPL results in the default sincos expansion rather than a vector call.
Sadly, this means all the LLVM IR tests pass, but it still is broken in Clang.
There was a problem hiding this comment.
This is why TargetOptions should go away
There was a problem hiding this comment.
AMDLIBM sincos still not working
There was a problem hiding this comment.
If you want that to work, can you add tests for all of the functions it it? We only have any test coverage for sleef and armpl. If there's no test, it's not going to work
There was a problem hiding this comment.
If you want that to work, can you add tests for all of the functions it it? We only have any test coverage for sleef and armpl. If there's no test, it's not going to work
AMDLIBM did have some tests
llvm-project/llvm/test/Transforms/LoopVectorize/X86/amdlibm-calls.ll
Lines 1760 to 1819 in 4d42a0c
There was a problem hiding this comment.
That's not the standard codegen usage that's broken. The fact that LoopVectorize directly emits these calls is actually really weird (#162239 (comment)).
What's missing is codegen tests for using these calls for legalization of the intrinsics. e.g., the important bit of SLEEF and ArmPL coverage is here:
https://github.com/llvm/llvm-project/blob/0fa6a67a4200ea1516f56e298df4a671af8a0642/llvm/test/CodeGen/AArch64/veclib-llvm.sincos.ll
https://github.com/llvm/llvm-project/blob/0fa6a67a4200ea1516f56e298df4a671af8a0642/llvm/test/CodeGen/AArch64/veclib-llvm.sincospi.ll
https://github.com/llvm/llvm-project/blob/0fa6a67a4200ea1516f56e298df4a671af8a0642/llvm/test/CodeGen/AArch64/veclib-llvm.modf.ll

Copy new process from sincospi.